Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Start/stop monitoring server based on monitoring config #3584

Merged
merged 12 commits into from
Oct 30, 2023
Merged

[Feature] Start/stop monitoring server based on monitoring config #3584

merged 12 commits into from
Oct 30, 2023

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented Oct 11, 2023 •
edited
Loading

due to cloud scenario where they have disabled monitoring but expect server to be running

this needs to be in sync with fleet to distinguish monitoring.enabled because we don't want to monitor anything and disabled because server should be down

Fixes: #2734

@michalpristas michalpristas added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team Team:Elastic-Agent Label for the Agent team backport-skip skip-changelog labels Oct 11, 2023
@michalpristas michalpristas self-assigned this Oct 11, 2023
@michalpristas michalpristas requested a review from a team as a code owner October 11, 2023 13:37
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 11, 2023 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-10-30T07:21:31.609+0000

  • Duration: 26 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 6613
Skipped 59
Total 6672

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 11, 2023 •
edited
Loading

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.837% (85/86) 👍 0.014
Files 66.993% (205/306) 👍 0.108
Classes 66.132% (371/561) 👍 0.121
Methods 53.169% (1166/2193) 👍 0.107
Lines 39.475% (13741/34809) 👍 0.079
Conditionals 100.0% (0/0) 💚

@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feat/relax-server upstream/feat/relax-server
git merge upstream/main
git push upstream feat/relax-server

@cmacknz
Copy link
Member

cmacknz commented Oct 11, 2023

I think we need to change the agent configuration used in cloud first, and since we are doing that we should address #2509 which will also need a cloud configuration change.

See #2734 (comment)

@michalpristas
Copy link
Contributor Author

this is dependent on work defined here: elastic/kibana#168629
kibana/fleet will signal 3 states (enabled, metrics, logs)

  • true, true, true: metrics and logs collection enabled
  • true, false, false: metrics and logs are not collected but server is running
  • false, false, false: metrics and logs are not collected, server is shutdown.

default behavior is either 1st or 3rd with very few exceptions which falls into seconds case.

@michalpristas michalpristas mentioned this pull request Oct 12, 2023
Closed
3 tasks
@michalpristas
Copy link
Contributor Author

all dependent work is done. bringing this alive

@michalpristas michalpristas changed the title start stop server decision based only on metrics.enabled [Feature] Start/stop monitoring server based on monitoring config Oct 23, 2023
AndersonQ
AndersonQ approved these changes Oct 23, 2023
View reviewed changes
internal/pkg/agent/application/monitoring/reload/reload.go Outdated
Comment on lines 70 to 75
func (sr *ServerReloader) Stop() error {
sr.isServerRunningLock.Lock()
defer sr.isServerRunningLock.Unlock()

return sr.stop()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion]
You could use an atomic.Bool instead of the mutex. I believe it'd make the code easier to maintain as it's not imediately obvious start() and stop() cannot lock the mutex because Reload() does so and then call them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

@michalpristas
Copy link
Contributor Author

failing on some unrelated tests, some of which are already addressed by @belimawr

@mergify
Copy link
Contributor

mergify bot commented Oct 27, 2023

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b feat/relax-server upstream/feat/relax-server
git merge upstream/main
git push upstream feat/relax-server

@elastic-sonarqube
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

61.0% 61.0% Coverage
0.0% 0.0% Duplication

@michalpristas michalpristas merged commit 66e0f95 into elastic:main Oct 30, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndersonQ AndersonQ AndersonQ approved these changes

@ycombinator ycombinator Awaiting requested review from ycombinator ycombinator is a code owner automatically assigned from elastic/elastic-agent-control-plane

Assignees

@michalpristas michalpristas

Labels
backport-skip skip-changelog Team:Elastic-Agent Label for the Agent team Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Elastic Agent always starts http monitoring service even if metrics are disabled
4 participants
@michalpristas @elasticmachine @cmacknz @AndersonQ